-
Notifications
You must be signed in to change notification settings - Fork 36
Add app insights and Logfire instrumentation #10
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR integrates Application Insights telemetry into the Expenses MCP server by combining Azure Monitor's OpenTelemetry instrumentation with Logfire's MCP tool call tracing. The implementation routes all telemetry data (from both Azure SDKs and MCP tool calls) to Application Insights.
Key changes:
- Added
azure-monitor-opentelemetry>=1.6.4andlogfire>=3.11.0dependencies - Configured OpenTelemetry tracing with both Azure Monitor and Logfire instrumentation in the server startup
- Added Application Insights infrastructure resource with conditional deployment based on
useMonitoringparameter
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| pyproject.toml | Added azure-monitor-opentelemetry and logfire dependencies for telemetry |
| uv.lock | Locked new dependencies including logfire 4.15.1 and related OpenTelemetry packages |
| servers/deployed_mcp.py | Configured OpenTelemetry tracing with Azure Monitor for SDK calls and Logfire for MCP tool instrumentation, added conditional monitoring setup based on connection string availability, adjusted logging levels |
| infra/main.bicep | Added Application Insights module with workspace integration and exposed connection string as output |
| infra/aca.bicep | Added APPLICATIONINSIGHTS_CONNECTION_STRING environment variable to container app configuration |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 5 out of 7 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 7 out of 9 changed files in this pull request and generated 9 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| # Configure OpenTelemetry tracing, either via Azure Monitor or Logfire | ||
| # We don't support both at the same time due to potential conflicts with tracer providers | ||
| if os.getenv("APPLICATIONINSIGHTS_CONNECTION_STRING"): |
Copilot
AI
Dec 3, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The conditional logic checks for the presence of environment variables using os.getenv() which returns None for unset variables but could also return empty strings. When useMonitoring is false in the Bicep deployment, APPLICATIONINSIGHTS_CONNECTION_STRING will be set to an empty string (see aca.bicep line 66), which would pass this truthy check and call configure_azure_monitor() with an invalid connection string. Update the condition to check for non-empty strings: if os.getenv("APPLICATIONINSIGHTS_CONNECTION_STRING"): should be if os.getenv("APPLICATIONINSIGHTS_CONNECTION_STRING") and os.getenv("APPLICATIONINSIGHTS_CONNECTION_STRING").strip():
| if os.getenv("APPLICATIONINSIGHTS_CONNECTION_STRING"): | |
| connection_string = os.getenv("APPLICATIONINSIGHTS_CONNECTION_STRING") | |
| if connection_string and connection_string.strip(): |
| f"tool.{tool_name}", | ||
| attributes={ | ||
| "mcp.method": context.method, | ||
| "mcp.source": context.source, | ||
| "mcp.tool.name": tool_name, | ||
| # If arguments are sensitive, consider omitting or sanitizing them | ||
| # If arguments are long/nested, consider adding a size or depth limit | ||
| "mcp.tool.arguments": str(context.message.arguments), | ||
| }, | ||
| ) as span: | ||
| try: | ||
| result = await call_next(context) | ||
| span.set_attribute("mcp.tool.success", True) | ||
| span.set_status(Status(StatusCode.OK)) | ||
| return result | ||
| except Exception as e: | ||
| span.set_attribute("mcp.tool.success", False) | ||
| span.set_attribute("mcp.tool.error", str(e)) | ||
| span.set_status(Status(StatusCode.ERROR, str(e))) | ||
| span.record_exception(e) | ||
| raise | ||
|
|
||
| async def on_read_resource(self, context: MiddlewareContext, call_next): | ||
| """Create a span for each resource read.""" | ||
| resource_uri = str(getattr(context.message, "uri", "unknown")) | ||
|
|
||
| with self.tracer.start_as_current_span( | ||
| f"resource.{resource_uri}", | ||
| attributes={ | ||
| "mcp.method": context.method, | ||
| "mcp.source": context.source, | ||
| "mcp.resource.uri": resource_uri, | ||
| }, | ||
| ) as span: | ||
| try: | ||
| result = await call_next(context) | ||
| span.set_attribute("mcp.resource.success", True) | ||
| span.set_status(Status(StatusCode.OK)) | ||
| return result | ||
| except Exception as e: | ||
| span.set_attribute("mcp.resource.success", False) | ||
| span.set_attribute("mcp.resource.error", str(e)) | ||
| span.set_status(Status(StatusCode.ERROR, str(e))) | ||
| span.record_exception(e) | ||
| raise | ||
|
|
||
| async def on_get_prompt(self, context: MiddlewareContext, call_next): | ||
| """Create a span for each prompt retrieval.""" | ||
| prompt_name = getattr(context.message, "name", "unknown") | ||
|
|
||
| with self.tracer.start_as_current_span( | ||
| f"prompt.{prompt_name}", |
Copilot
AI
Dec 3, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The span naming pattern f"tool.{tool_name}", f"resource.{resource_uri}", and f"prompt.{prompt_name}" could lead to high cardinality issues if this middleware is reused in contexts where these identifiers contain dynamic values (e.g., UUIDs, timestamps). For better observability practices, consider using fixed span names (e.g., "tool.call", "resource.read", "prompt.get") with the specific identifier stored only as an attribute, which is already being done.
| if os.getenv("APPLICATIONINSIGHTS_CONNECTION_STRING"): | ||
| logger.info("Setting up Azure Monitor instrumentation") | ||
| configure_azure_monitor() | ||
| elif os.getenv("LOGFIRE_PROJECT_NAME"): | ||
| logger.info("Setting up Logfire instrumentation") | ||
| settings.tracing_implementation = "opentelemetry" # Configure Azure SDK to use OpenTelemetry tracing |
Copilot
AI
Dec 3, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Azure SDK tracing implementation setting (settings.tracing_implementation = "opentelemetry") is only configured when using Logfire, but not when using Azure Monitor. This could lead to inconsistent Azure SDK tracing behavior between the two observability platforms. Consider setting this configuration before the conditional blocks (line 33) to ensure Azure SDK always uses OpenTelemetry tracing regardless of the export destination.
| if os.getenv("APPLICATIONINSIGHTS_CONNECTION_STRING"): | |
| logger.info("Setting up Azure Monitor instrumentation") | |
| configure_azure_monitor() | |
| elif os.getenv("LOGFIRE_PROJECT_NAME"): | |
| logger.info("Setting up Logfire instrumentation") | |
| settings.tracing_implementation = "opentelemetry" # Configure Azure SDK to use OpenTelemetry tracing | |
| settings.tracing_implementation = "opentelemetry" # Ensure Azure SDK always uses OpenTelemetry tracing | |
| if os.getenv("APPLICATIONINSIGHTS_CONNECTION_STRING"): | |
| logger.info("Setting up Azure Monitor instrumentation") | |
| configure_azure_monitor() | |
| elif os.getenv("LOGFIRE_PROJECT_NAME"): | |
| logger.info("Setting up Logfire instrumentation") |
| elif os.getenv("LOGFIRE_PROJECT_NAME"): | ||
| logger.info("Setting up Logfire instrumentation") | ||
| settings.tracing_implementation = "opentelemetry" # Configure Azure SDK to use OpenTelemetry tracing | ||
| logfire.configure(service_name="expenses-mcp", send_to_logfire=True) |
Copilot
AI
Dec 3, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The send_to_logfire=True parameter is redundant when LOGFIRE_PROJECT_NAME is set. The logfire.configure() function automatically enables sending to Logfire when a project name is configured. This parameter should be omitted for clarity.
| logfire.configure(service_name="expenses-mcp", send_to_logfire=True) | |
| logfire.configure(service_name="expenses-mcp") |
| "mcp.tool.name": tool_name, | ||
| # If arguments are sensitive, consider omitting or sanitizing them | ||
| # If arguments are long/nested, consider adding a size or depth limit | ||
| "mcp.tool.arguments": str(context.message.arguments), |
Copilot
AI
Dec 3, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potential security concern: Tool arguments are logged to OpenTelemetry spans as-is with str(context.message.arguments). This could expose sensitive data (e.g., amounts, descriptions, personal information) to the observability platform. Consider implementing a sanitization strategy or providing configuration to exclude sensitive fields from telemetry data. The comment on line 22-23 acknowledges this but doesn't implement a solution.
| "agent-framework>=1.0.0b251016", | ||
| "azure-cosmos>=4.9.0", | ||
| "azure-monitor-opentelemetry>=1.6.4", | ||
| "opentelemetry-instrumentation-starlette>=0.49b0", |
Copilot
AI
Dec 3, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The version constraint >=0.49b0 for opentelemetry-instrumentation-starlette doesn't match the actual version in uv.lock which is 0.59b0. Consider updating the constraint to >=0.59b0 to match the locked version and ensure compatibility, or use a more specific constraint like >=0.59b0,<0.60 to prevent unexpected breaking changes from beta versions.
| "opentelemetry-instrumentation-starlette>=0.49b0", | |
| "opentelemetry-instrumentation-starlette>=0.59b0,<0.60", |
| { | ||
| name: 'APPLICATIONINSIGHTS_CONNECTION_STRING' | ||
| value: applicationInsightsConnectionString | ||
| } |
Copilot
AI
Dec 3, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When Application Insights connection string is empty (when useMonitoring is false), the environment variable APPLICATIONINSIGHTS_CONNECTION_STRING will be set to an empty string in the container app. This could potentially trigger the Azure Monitor instrumentation setup in deployed_mcp.py (line 35-37) with an invalid connection string, leading to errors. Consider either not adding the environment variable when it's empty, or checking for non-empty values in the Python code before calling configure_azure_monitor().
| { | |
| name: 'APPLICATIONINSIGHTS_CONNECTION_STRING' | |
| value: applicationInsightsConnectionString | |
| } | |
| // Only add the environment variable if the connection string is non-empty | |
| ...(empty(applicationInsightsConnectionString) ? [] : [ | |
| { | |
| name: 'APPLICATIONINSIGHTS_CONNECTION_STRING' | |
| value: applicationInsightsConnectionString | |
| } | |
| ]) |
| // We typically do not output sensitive values, but App Insights connection strings are not considered highly sensitive | ||
| output APPLICATIONINSIGHTS_CONNECTION_STRING string = useMonitoring ? applicationInsights.outputs.connectionString : '' |
Copilot
AI
Dec 3, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
APPLICATIONINSIGHTS_CONNECTION_STRING is being exported as a top-level output, which can be viewed by anyone with access to deployment outputs and may be logged or surfaced in tooling. This connection string includes the instrumentation key and can be used to submit telemetry to your Application Insights resource, making it sensitive. Mitigation: avoid outputting this value or mark it as a secure output; instead fetch it securely at runtime or store it in a secret store (e.g., Key Vault) and reference it from deployments.
| // We typically do not output sensitive values, but App Insights connection strings are not considered highly sensitive | |
| output APPLICATIONINSIGHTS_CONNECTION_STRING string = useMonitoring ? applicationInsights.outputs.connectionString : '' | |
| // Application Insights connection string is sensitive and should not be output as a deployment output. |
| value: applicationInsightsConnectionString | ||
| } |
Copilot
AI
Dec 3, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
APPLICATIONINSIGHTS_CONNECTION_STRING is set as a plain environment variable for the container app, increasing chances of accidental exposure via logs, crash dumps, or diagnostics endpoints. Attackers with limited foothold (e.g., read access to env or logs) could exfiltrate it and push malicious telemetry. Fix: store this value as a secret (Container Apps secrets or Key Vault) and reference it from the app; avoid placing it directly in environmentVariables.
This PR adds observability with OpenTelemetry, supporting export to both Azure App Insights (Azure Monitor) and Logfire.
My approach:
Logfire:
Logfire with Starlette spans:
App Insights:
App Insights with Starlette spans: